Conversation
📝 WalkthroughWalkthroughStickerify now implements AutoCloseable and adds a public close() to remove listeners, shut down executors and the Telegram bot. Main is changed to use try-with-resources, a LOCK to block the main thread, and a shutdown hook to notify LOCK. Tests and mocks updated to use resource-based helpers and runBot() returning Stickerify. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor JVM as JVM
participant Main as Main
participant Stickerify as Stickerify
participant TG as Telegram API
Note over Main: Startup
Main->>Stickerify: new Stickerify(...)
activate Stickerify
Stickerify->>TG: setUpdatesListener(...)
Main->>Main: synchronized(LOCK) { LOCK.wait() }
Note over JVM,Main: JVM shutdown or external notify
JVM->>Main: shutdown hook → notify LOCK
alt Interrupted
Main-->>Main: catch InterruptedException → Thread.currentThread().interrupt()
end
Note over Main,Stickerify: try-with-resources exit
Main->>Stickerify: close()
Stickerify->>TG: removeUpdatesListener()
Stickerify->>Stickerify: shutdown executor (if ExecutorService)
Stickerify->>TG: shutdown bot
deactivate Stickerify
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labelsenhancement, unit test Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java (1)
94-98: Close executor (when applicable) and order shutdown safely.Stop accepting updates, let in‑flight tasks finish, then shut the bot. Prevents races and stray tasks.
@Override public void close() { - bot.removeGetUpdatesListener(); - bot.shutdown(); + bot.removeGetUpdatesListener(); + if (executor instanceof java.util.concurrent.ExecutorService es) { + es.close(); // waits for tasks to complete + } + bot.shutdown(); }src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java (1)
34-40: Avoid indefinite test hangs by using timed takeRequest.Optional but safer: use the timeout overload to fail fast when expectations aren’t met.
- var getUpdates = server.takeRequest(); + var getUpdates = server.takeRequest(5, java.util.concurrent.TimeUnit.SECONDS); assertEquals("/api/token/getUpdates", getUpdates.getTarget()); - var sendMessage = server.takeRequest(); + var sendMessage = server.takeRequest(5, java.util.concurrent.TimeUnit.SECONDS);Apply similarly to other takeRequest() calls.
src/test/java/com/github/stickerifier/stickerify/bot/MockResponses.java (1)
10-14: Return an empty result array to mirror Telegram’s shape.Current body lacks result; some parsers expect
result: []. Improves fidelity and reduces flakiness.static final MockResponse EMPTY_UPDATES = new MockResponse.Builder().body(""" { - ok: true + ok: true, + result: [] } """).build();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java(2 hunks)src/main/java/com/github/stickerifier/stickerify/runner/Main.java(1 hunks)src/test/java/com/github/stickerifier/stickerify/bot/MockResponses.java(3 hunks)src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java (1)
src/test/java/com/github/stickerifier/stickerify/bot/MockResponses.java (1)
MockResponses(8-345)
src/test/java/com/github/stickerifier/stickerify/bot/MockResponses.java (1)
src/test/java/com/github/stickerifier/stickerify/ResourceHelper.java (1)
ResourceHelper(7-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Execute unit tests (macos-13)
- GitHub Check: Execute unit tests (windows-latest)
- GitHub Check: Execute unit tests (ubuntu-latest)
🔇 Additional comments (4)
src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java (1)
43-53: Deterministic test setup looks good.Using QueueDispatcher.failFast plus Runnable::run keeps tests synchronous and stable.
src/test/java/com/github/stickerifier/stickerify/bot/MockResponses.java (1)
322-332: API refactor in helpers looks correct.Switch to filename‑based helpers and ResourceHelper usage is clean and cohesive.
Also applies to: 334-340
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java (1)
51-51: Approve — Stickerify is AutoCloseable; verify there are no remaining non‑try‑with‑resources call sites.Could not confirm automatically: the supplied ripgrep output was "No files were searched". Run from the repo root:
rg -nP '\bnew\s+Stickerify\s*(' -g '!/target/'
or
git grep -n "new Stickerify(" -- '*.java'If matches are returned, convert usages to try-with-resources or ensure callers call close().
src/main/java/com/github/stickerifier/stickerify/runner/Main.java (1)
9-9: Compilation blocker: '_' is a reserved identifier.Java forbids single underscore as a variable name. Rename.
- try (var _ = new Stickerify()) { + try (var ignored = new Stickerify()) {⛔ Skipped due to learnings
Learnt from: MartelliEnrico PR: Stickerifier/Stickerify#397 File: src/main/java/com/github/stickerifier/stickerify/media/MediaHelper.java:424-427 Timestamp: 2025-08-31T12:07:44.464Z Learning: In Java 22+, the underscore character '_' is the correct and recommended way to declare unnamed variables in catch clauses, lambda parameters, and local variable declarations. JEP 456 "Unnamed Variables & Patterns" finalized this feature, making '_' the official keyword for unused variables that cannot be referenced after declaration.
src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java (1)
95-104: Prefer graceful executor shutdown overclose()to avoid abandoning in‑flight tasksCalling
ExecutorService#close()typically maps toshutdown()without awaiting completion. To reduce flakiness and background errors, wait briefly for tasks to finish and then force shutdown if needed.Apply:
@@ -import java.util.concurrent.ExecutorService; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; @@ @Override public void close() { bot.removeGetUpdatesListener(); - if (executor instanceof ExecutorService es) { - es.close(); - } + if (executor instanceof ExecutorService es) { + es.shutdown(); + try { + if (!es.awaitTermination(5, TimeUnit.SECONDS)) { + es.shutdownNow(); + } + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + } + } bot.shutdown(); }Also applies to: 44-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java(2 hunks)src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java (1)
src/test/java/com/github/stickerifier/stickerify/bot/MockResponses.java (1)
MockResponses(8-345)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java (1)
src/main/java/com/github/stickerifier/stickerify/runner/Main.java (1)
Main(5-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Execute unit tests (macos-13)
- GitHub Check: Execute unit tests (ubuntu-latest)
- GitHub Check: Execute unit tests (windows-latest)
🔇 Additional comments (5)
src/main/java/com/github/stickerifier/stickerify/bot/Stickerify.java (1)
52-52: Lifecycle integration via AutoCloseable — LGTMImplementing AutoCloseable on the record cleanly enables try‑with‑resources for tests and Main.
src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java (4)
14-14: Import of QueueDispatcher — LGTMRequired for
setFailFastusage below.
30-33: Fail‑fast dispatcher setup — LGTMInitialising the dispatcher in
@BeforeEachremoves flakiness from background polling.
49-57:runBot()returning AutoCloseable Stickerify — LGTMReturning a
Stickerifyinstance and usingRunnable::runkeeps tests deterministic and leverages the newclose()lifecycle.
39-46: Compilation blocker:_is an illegal identifier in Java ≥9 — rename the try‑with‑resources variableThis will not compile on modern JDKs. Use a valid name (e.g.,
ignored) in all occurrences.Example fix (apply to all shown occurrences):
- try (var _ = runBot()) { + try (var ignored = runBot()) {#!/bin/bash # Verify that no illegal '_' resource identifiers remain rg -nP --type=java -C1 $'try\\s*\\(\\s*var\\s+_\\s*=\\s*runBot\\(\\)\\s*\\)' src/testAlso applies to: 69-76, 83-90, 97-104, 111-118, 127-142, 151-168, 176-193, 201-218, 226-243, 251-268, 276-292, 300-316
rob93c
added
enhancement
src/test/java/com/github/stickerifier/stickerify/bot/StickerifyTest.java
Outdated
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests